fix(templating)!: return json, not go structs #12909
Conversation
Signed-off-by: Baris Erdem <[email protected]>
Signed-off-by: Baris Erdem <[email protected]>
Signed-off-by: Baris Erdem <[email protected]>
Signed-off-by: Baris Erdem <[email protected]>
agilgur5
changed the title
Signed-off-by: Baris Erdem <[email protected]>
|
Hello @agilgur5 I think I need another approval to get this merged. :) |
|
This is a breaking change from previous bad but usable behaviour. I am not sure whether this should be held until 3.6. If it goes in 3.5.6 it should be highlighted in the notes. |
|
@Joibel but did the previous behavior ever make sense / was it ever useful? From a quick glance, it sounds like it would always cause an error, so then we could straightforwardly merge this as a patch and wouldn't even necessarily need notes as the previous behavior did not work. I feel like there might have been a use for this behavior before but I couldn't think of one off the top of my head. Using deep, nested structures without a pre-parsing step was not a use-case I had used much though. |
|
The previous behaviour didn't result in an error, was deterministic and predictable (you got the go printf This could have been used, it contains the values you want, just not in the representation expected. I wouldn't be surprised if someone somewhere does rely on this, and so we should point out that we are changing it. |
Hi! As far as I tested, nothing changes for simple values. For complex values, since it returns go struct, it's not usable when you pass to the next steps(I'm not sure if there are cases where it works fine, though). Even if it causes some issues for some people, it looks like an easy change to do. 🤔 Lastly, do you have an idea to make sure we don't break anything? cc: @agilgur5 |
|
I am happy to merge this and put something into the release notes. It was wrong, but didn't error and was usable, so wanted to call that out. |
I also think that way. Thanks! Who should I ask for additional approval that gives me the right to merge this PR then? 🤔 |
|
I'll merge it now, the checks weren't green, but they are now. |
|
Just re-stumbled upon #8930, which includes workarounds users have been using for some time that we would not want to break. Also if it does, we should really add the conventional |
Joibel
changed the title
|
Notes for maintainers: this will come out in 3.6 and should be noted in the release notes. Please don't backport to any other branches. |
agilgur5
changed the title
|
Mmm if this is going to be a breaking change, the release note should really have been included in this PR too -- we already have some for 3.6 on |
agilgur5
added
the
solution/workaround
|
@agilgur5 Hi, I’ll look into this next week. |
|
@Joibel @isubasinghe could one of you take over the release notes and test cases for this? The cases I mentioned above of expressions on structs such as in #8930 and #12739 in particular need test cases and need checking for breakage. |
|
I wrote up a follow-up issue to track the above in #13541 |
Motivation
When you extract a value from json using jsonpath expressions it returns values properly, only for simple values. For instance when you pass the following JSON:
{"employees": [{"name": "Baris", "age":43},{"name": "Mo", "age": 42}, {"name": "Jai", "age" :44}]}and$.employees[0].nameit'll return "Baris" correctly. But let's say if you want to return an item in the array, for instance with$.employees[0]query then you'll getmap[age:43 name:Baris]. This is not correct, it returns internal representation of the value when returned value is not a simple value.We're passing some complex values to each steps and some of the values returned from a jsonpath expression could be a complex object. So we face with issues and doing some workarounds to get this correctly. Instead I thought maybe it'd be useful to fix it directly here. With this PR we'll be able to get values correctly from jsonpath expressions. For instance
$.employees[0]now would return:{"age":43,"name":"Baris"}Modifications
The returned value is now passed to
json.Marshaldirectly without formatting it.fmt.Sprintf("%v", result))this actually causes getting internal representation of the value.The rest of the code is making sure we're returning a correct json value/object, mainly related to quotes. The changes I did in this PR is to cover when the returned object is a complex object and it contains strings, so we'll have quotes inside of the value. Then we need to properly encode them.
Verification
Please let me know if there's anything missing or something I can improve more. Thanks!